-
Notifications
You must be signed in to change notification settings - Fork 122
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
No needless dropping of 'estimated_town_class' #602
Conversation
8a7a4ea
to
054dcd6
Compare
To enable the crossing of a city along rivers, only one line of code in the profile text needs to be adjusted: switch estimated_town_class= 0 -> switch or estimated_town_class= ( not estimated_river_class= ) 0
054dcd6
to
8721802
Compare
@afischerdev, have you thought about it yet? |
As before, I don't mind to do that. But I thought maybe @EssBee59 would like to add something else to that. |
@EssBee59 |
Hello, |
The proposed change does not "not clear the town penalty". It only shifts the place where the mingling between the two tags is happening from the preprocessing into to the profile, which increases the usability of the new tags because we no longer enforce one preference (namely your preference) upon users.
Yes, but without this change users simply cannot decide if they want to intermix the river and town class or not!
So what is the problem if you can simply replace
You completely ignore that the proposed change increases the freedom of choice for profile developers. |
@afischerdev, I'm not going to accept @EssBee59 answer. We currently spend a lot of computational time figuring out which way is within a city, only to later discard some of that information. This pull request is not about "own desires" but about reasonable design decisions and throwing this valuable information away for no good reason is unreasonable. Especially when avoiding cities is likely going to be on if not the biggest use-case of the new tags. |
@quaelnix But back to the facts:
The arguments are understandable.
The sample issue #608 is not perfect for this discussion:
@quaelnix |
Exactly.
Again, it makes no sense to drop valuable information because of space concern.
Which arguments?
is not an argument. |
Usually some of the first things users expect from routers and they don't find in BRouter So additional data in the database that will make users' lives easier, |
There is a branch on my BRouter fork that generates town tags based on building density: master...quaelnix:brouter:improve-town-and-forest-tags And there are a couple of comments that contain examples here: #486 (comment) But aside from the considerable performance impact this will also not work properly in countries with poor OSM data, which is why I did not follow through on that concept. My best concept for the town tag so far can be found here: #614 I would love to discuss the pros and cons of the different approaches and find the best solution as a team, but there doesn't seem to be much interest in taking the new pseudo tags to the next level. Which is a bit sad to be honest. |
I see the street name problem, but I doesn't see the others. |
@quaelnix I also tried #614, it runs much faster. But it's broken down to 1000 residents per village. I can't say what that means to database. May be @EssBee59 have more experiences here. May be a whole world test could be done. |
But take a look at cities like Sydney, Australia, where the majority of buildings are still missing.
I'm pretty sure that a SQL expert would be able to speed up our sql code by a factor of 2 or more. |
Try this route with the Then toggle |
@quaelnix |
To enable the crossing of a city along rivers, only one line of code in the profile text needs to be adjusted:
->
This significantly improves the usability of the
estimated_town_class
pseudo tag.